Don't treat non-success HTTP codes as transport errors#486
Don't treat non-success HTTP codes as transport errors#486SteffenDE wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
The spec states that:
> If the server cannot accept the input, it MUST return an HTTP error
> status code (e.g., 400 Bad Request).
> The HTTP response body MAY comprise a JSON-RPC error response that has no id.
But the rust-sdk treated any POST responses with non successful status
as an error.
Before this patch, trying to call list_resources on a server that does
return HTTP 400 with JSON-RPC error:
```
TransportSend(
DynamicTransportError {
transport_name: "rmcp::transport::worker::WorkerTransport<rmcp::transport::streamable_http_client::StreamableHttpClientWorker<reqwest::async_impl::client::Client>>",
transport_type_id: TypeId(0xf7bacf3cf3889d471ead32d7a3cc8155),
error: Client(
reqwest::Error {
kind: Status(
400,
None,
),
url: "http://localhost:4000/mcp",
},
),
},
)
```
After this patch:
```
McpError(
ErrorData {
code: ErrorCode(
-32601,
),
message: "Method not found",
data: Some(
Object {
"name": String("resources/list"),
},
),
},
)
```
There was a problem hiding this comment.
Pull Request Overview
This PR fixes HTTP transport error handling to properly distinguish between transport-level failures and application-level JSON-RPC errors returned via HTTP error status codes, aligning with the MCP specification.
- Removes
error_for_status()calls that incorrectly treated HTTP error codes as transport errors - Allows JSON-RPC error responses with HTTP 4xx/5xx status codes to be parsed and returned as proper MCP errors
- Enables clients to receive server-sent error details instead of generic HTTP transport errors
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
crates/rmcp/src/transport/common/reqwest/streamable_http_client.rs |
Removes error_for_status() check to allow non-2xx responses to be processed as potential JSON-RPC errors |
crates/rmcp/src/transport/common/reqwest/sse_client.rs |
Removes error_for_status() check for SSE transport consistency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Good catch, but should we have some log when response status is not success? |
|
I don’t think there’s a need to log, since it’s part of the spec to have non-200 codes and a client will still receive this as an error, so users can decide to log if they want. |
alexhancock
left a comment
There was a problem hiding this comment.
It LGTM. Sorry for the delay in getting to it.
@SteffenDE Do you mind rebasing and having the commit message conform to https://www.conventionalcommits.org, then we can merge.
|
@alexhancock No worries! We're still using the SSE code, so I don't want to update that branch for now. As it's only one line to remove I think it's fine if I just close the PR and you do that change directly :) |
The spec states that:
But the rust-sdk treated any POST responses with non successful status as an error.
Before this patch, trying to call list_resources on a server that does return HTTP 400 with JSON-RPC error:
After this patch:
Motivation and Context
Clients should be able to see the error the server sends. The code already checks for JSON content type, I assume the error_for_status for included by accident.
How Has This Been Tested?
Tests still seem to pass, but I did not add a new one. For testing, I created a demo client, but someone with more knowledge of the code should add a proper test case.
Breaking Changes
Could be seen as a breaking change if people relied on getting the transport error.
Types of changes
Checklist
Additional context